Specify User Agent for Console CLI requests #952

lewijacn · 2024-09-12T15:25:13Z

Description

Allows setting a global user agent on CDK deployments that will be configured in a client_options section in the services.yaml. This user agent will currently be provided to the Traffic Replayer for its replayed requests (existing logic) and to the Console CLI for all of its boto3 requests and python requests library requests. This can also be extended to other migrations as support is added.

Issues Resolved

https://opensearch.atlassian.net/browse/MIGRATIONS-1835

Testing

Unit testing and Cloud deployment testing

Check List

New functionality includes testing
- All tests pass, including unit test, integration test and doctest
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

codecov · 2024-09-12T15:28:47Z

Codecov Report

Attention: Patch coverage is 93.68421% with 6 lines in your changes missing coverage. Please review.

Project coverage is 78.98%. Comparing base (492e5b9) to head (9c8868c).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...console_link/console_link/models/metrics_source.py	78.57%	3 Missing ⚠️
...sole/lib/console_link/console_link/models/utils.py	90.47%	2 Missing ⚠️
.../lib/console_link/console_link/models/osi_utils.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #952      +/-   ##
============================================
- Coverage     80.88%   78.98%   -1.90%     
  Complexity     2723     2723              
============================================
  Files           402      375      -27     
  Lines         15586    13869    -1717     
  Branches        970      970              
============================================
- Hits          12607    10955    -1652     
+ Misses         2405     2340      -65     
  Partials        574      574

Flag	Coverage Δ
gradle-test	`76.80% <ø> (ø)`
python-test	`89.90% <93.68%> (-2.86%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

...tion/src/main/docker/migrationConsole/lib/console_link/console_link/models/client_options.py

mikaylathompson · 2024-09-18T13:52:27Z

...rSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/factories.py

@@ -1,6 +1,7 @@
 from enum import Enum
 from typing import Dict, Optional

+from console_link.models.client_options import ClientOptions


Okay, I think I slightly regret YAML vs env variable decision because we could have kept env variable stuff much more limited to only grabbing it at the level of each client, instead of having to pass it all the way through the code -- it feels like conceptually this shouldn't have to be woven through all these factories and lots of unrelated code and it should just stay low level, but I don't really have any way in mind to implement that if it comes from the YAML.

Which is all to say, I'm not sure if there's a better solution or not, and definitely not sure whether it's worth refactoring this, because I think it's well-implemented for this route. Thoughts?

(Circling back to add that after looking at the tests, this services.yaml approach is definitely clearer and easier to test, so that's a point in its favor)

From some discussion with Mikayla there are pros and cons with the current approach versus setting something more globally instead of passing through yaml. I'm comfortable with the current approach for now, and think it can be improved so there are less touch points next time we want some global settings but let me know if you have concerns @mikaylathompson

mikaylathompson · 2024-09-18T13:54:16Z

...tion/src/main/docker/migrationConsole/lib/console_link/console_link/models/metrics_source.py

@@ -217,9 +216,14 @@ def get_metrics(self, recent=False) -> Dict[str, List[str]]:
            raise NotImplementedError("Recent metrics are not implemented for Prometheus")
        for c in Component:
            exported_job = prometheus_component_names(c)
+            headers = None
+            if self.client_options and self.client_options.user_agent_extra:
+                headers = append_user_agent_header_for_requests(headers=None,


Out of curiosity, what's the reasoning for adding it to the headers here? I guess that if a user is using this field for something other than what we are, they would expect it to be applied for more than just aws services?

Yes exactly, I want this user-agent feature to be valuable for other use cases cx may have as well

mikaylathompson · 2024-09-18T13:59:07Z

...rSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/osi_utils.py

+    if source_cluster.auth_details and source_cluster.auth_details["password_from_secret_arn"]:
+        source_auth_secret = source_cluster.auth_details["password_from_secret_arn"]


this is just the arn -- should it be the real password? If so, it needs to use the method that pulls it from secrets manager. Additionally, if password_from_secret_arn doesn't exist, line 288 will throw a key error (python vs js semantics)

Suggested change

if source_cluster.auth_details and source_cluster.auth_details["password_from_secret_arn"]:

source_auth_secret = source_cluster.auth_details["password_from_secret_arn"]

if source_cluster.auth_details and "password_from_secret_arn" in source_cluster.auth_details:

source_auth_secret = source_cluster.get_basic_auth_password()

Oh, actually, source_auth_secret does sound like you just want the arn. So ignore the second line!

Thanks for catching, have updated now. And yes we want the arn here 👍

mikaylathompson · 2024-09-18T14:01:01Z

...ockerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/utils.py

+    adjusted_headers = dict(headers) if headers else {}
+    if "User-Agent" in adjusted_headers:
+        adjusted_headers["User-Agent"] = f"{adjusted_headers['User-Agent']} {user_agent_extra}"


The annotation says these aren't covered by a test, but I think this would be a very good condition to have an explicit check on. Can you add a quick one?

mikaylathompson · 2024-09-18T14:01:30Z

...ockerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/utils.py

+        adjusted_headers["User-Agent"] = f"{requests.utils.default_user_agent()} {user_agent_extra}"
+    return adjusted_headers


Actually, these lines aren't covered either, so maybe two tests that cover this whole function?

mikaylathompson · 2024-09-18T14:04:27Z

deployment/cdk/opensearch-service-migration/lib/migration-services-yaml.ts

@@ -122,6 +122,10 @@ export class KafkaYaml {
    standard?: string | null;
 }

+export class ClientOptions {
+    user_agent_extra?: string | null;


nit, but doesn't the ? mean that you don't have to add | null?

This is a good point, I've seen us use this pattern but do we ever use null? Just having the ? is sufficient for undefined. I have removed the null and not seeing any issues with tests currently

# Conflicts: # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/cluster.py # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/metrics_source.py # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/utils.py

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

# Conflicts: # deployment/cdk/opensearch-service-migration/bin/app.ts # deployment/cdk/opensearch-service-migration/lib/stack-composer.ts

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

mikaylathompson

I have one more test request for something that includes a cluster call_api. Otherwise LGTM

...kerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/cluster.py

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

lewijacn added 4 commits September 11, 2024 15:18

Add basic user agent support for console cli

67a196c

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Link up remaining user agent appendages

3fa9a89

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Add link in CDK

cf1a5ba

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Flake8 fix

1219506

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

lewijacn added 3 commits September 12, 2024 13:27

Merge remote-tracking branch 'origin/main' into user-agent-console

712a256

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Add test cases

662d28a

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Flake8 fixes

796d6b2

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

lewijacn marked this pull request as ready for review September 12, 2024 21:19

lewijacn requested review from AndreKurait, chelma, gregschohn, mikaylathompson, okhasawn, peternied and sumobrian as code owners September 12, 2024 21:19

mikaylathompson reviewed Sep 18, 2024

View reviewed changes

lewijacn added 8 commits September 19, 2024 10:55

Merge remote-tracking branch 'origin/main' into user-agent-console

352f23b

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

PR feedback and add some unit tests for utils

5c01347

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Merge remote-tracking branch 'origin/main' into user-agent-console

a733a61

# Conflicts: # deployment/cdk/opensearch-service-migration/bin/app.ts # deployment/cdk/opensearch-service-migration/lib/stack-composer.ts

Fix user agent from merge conflict

3cc59c0

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

PR feedback updates

c5c69dd

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Add tests for create boto3 client

346ef9c

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Flake8 fixes

21724b9

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

mikaylathompson approved these changes Sep 20, 2024

View reviewed changes

...kerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/cluster.py Show resolved Hide resolved

Add additional tests for client options

9c8868c

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

lewijacn merged commit 62d38b5 into opensearch-project:main Sep 20, 2024
12 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify User Agent for Console CLI requests #952

Specify User Agent for Console CLI requests #952

lewijacn commented Sep 12, 2024

codecov bot commented Sep 12, 2024 •

edited

Loading

mikaylathompson Sep 18, 2024

mikaylathompson Sep 18, 2024

lewijacn Sep 20, 2024

mikaylathompson Sep 18, 2024

lewijacn Sep 20, 2024

mikaylathompson Sep 18, 2024

mikaylathompson Sep 18, 2024

lewijacn Sep 20, 2024

mikaylathompson Sep 18, 2024

lewijacn Sep 20, 2024

mikaylathompson Sep 18, 2024

lewijacn Sep 20, 2024

mikaylathompson Sep 18, 2024

lewijacn Sep 19, 2024

mikaylathompson left a comment

		if source_cluster.auth_details and source_cluster.auth_details["password_from_secret_arn"]:
		source_auth_secret = source_cluster.auth_details["password_from_secret_arn"]

		adjusted_headers["User-Agent"] = f"{requests.utils.default_user_agent()} {user_agent_extra}"
		return adjusted_headers

Specify User Agent for Console CLI requests #952

Specify User Agent for Console CLI requests #952

Conversation

lewijacn commented Sep 12, 2024

Description

Issues Resolved

Testing

Check List

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaylathompson left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 12, 2024 •

edited

Loading